Allow refreshing the mirrors under IMP=false. #973

gouttegd · 2023-12-20T20:22:06Z

This PR overhauls the mirroring pipeline to make it independent of the IMP variable, so that refreshing the mirrors is controlled by the MIR variable only (except for “large” imports, for which the IMP_LARGE variable also comes into play).

This is done using Make conditionals rather than shell conditionals. This makes the generated Makefile much easier to read, as the entire mirroring pipeline is simply skipped (with a single conditional at the top) under MIR=false.

closes #946

This commit overhauls the mirroring pipeline to make it independent of the IMP variable, so that refreshing the mirrors is controlled by the MIR variable only (except for "large" imports, for which the IMP_LARGE variable also comes into play). This is done using Make conditionals rather than shell conditionals. This makes the generated Makefile much easier to read, as the entire mirroring pipeline is simply skipped (with a single conditional at the top) under MIR=false.

gouttegd · 2023-12-20T20:26:40Z

@matentzn Summarily tested on FBbt.

The Jinja2 template is borderline unreadable, so to understand better what the PR does, here is an example of the mirroring section of the generated Makefile:

# ----------------------------------------
# Mirroring upstream ontologies
# ----------------------------------------

IMP=true # Global parameter to bypass import generation
MIR=true # Global parameter to bypass mirror generation
IMP_LARGE=true # Global parameter to bypass handling of large imports

ifeq ($(strip $(MIR)),true)


## ONTOLOGY: ro
.PHONY: mirror-ro
.PRECIOUS: $(MIRRORDIR)/ro.owl
mirror-ro: | $(TMPDIR)
	curl -L $(OBOBASE)/ro.owl --create-dirs -o $(TMPDIR)/ro-download.owl --retry 4 --max-time 200 && \
	$(ROBOT) remove -i $(TMPDIR)/ro-download.owl --base-iri http://purl.obolibrary.org/obo/BFO_ --base-iri http://purl.obolibrary.org/obo/RO_ --axioms external --preserve-structure false --trim false -o $(TMPDIR)/$@.owl


## ONTOLOGY: bfo
.PHONY: mirror-bfo
.PRECIOUS: $(MIRRORDIR)/bfo.owl
mirror-bfo: | $(TMPDIR)
	curl -L $(OBOBASE)/bfo.owl --create-dirs -o $(TMPDIR)/bfo-download.owl --retry 4 --max-time 200 && \
	$(ROBOT) convert -i $(TMPDIR)/bfo-download.owl -o $(TMPDIR)/$@.owl


## ONTOLOGY: pato
.PHONY: mirror-pato
.PRECIOUS: $(MIRRORDIR)/pato.owl
ifeq ($(strip $(IMP_LARGE)),true)
mirror-pato: | $(TMPDIR)
	curl -L $(OBOBASE)/pato.owl --create-dirs -o $(TMPDIR)/pato-download.owl --retry 4 --max-time 200 && \
	$(ROBOT) remove -i $(TMPDIR)/pato-download.owl --base-iri $(OBOBASE)/PATO --axioms external --preserve-structure false --trim false -o $(TMPDIR)/$@.owl
else
mirror-pato:
	@echo "Not refreshing pato because refreshing large imports is disabled (IMP_LARGE=$(IMP_LARGE))."
endif

ALL_MIRRORS = $(patsubst %, $(MIRRORDIR)/%.owl, $(IMPORTS))
MERGE_MIRRORS = true

ifeq ($(strip $(MERGE_MIRRORS)),true)
$(MIRRORDIR)/merged.owl: $(ALL_MIRRORS)
	$(ROBOT) merge $(patsubst %, -i %, $^)  remove --axioms equivalent --preserve-structure false -o $@
.PRECIOUS: $(MIRRORDIR)/merged.owl
endif


$(MIRRORDIR)/%.owl: mirror-% | $(MIRRORDIR)
	if [ -f $(TMPDIR)/mirror-$*.owl ]; then if cmp -s $(TMPDIR)/mirror-$*.owl $@ ; then echo "Mirror identical, ignoring."; else echo "Mirrors different, updating." &&\
		cp $(TMPDIR)/mirror-$*.owl $@; fi; fi

else # MIR=false
$(MIRRORDIR)/%.owl:
	@echo "Not refreshing $@ because the mirrorring pipeline is disabled (MIR=$(MIR))."
endif

to compare with what the same section generated by ODK 1.4.3:

# ----------------------------------------
# Mirroring upstream ontologies
# ----------------------------------------

IMP=true # Global parameter to bypass import generation
MIR=true # Global parameter to bypass mirror generation
IMP_LARGE=true # Global parameter to bypass handling of large imports



## ONTOLOGY: ro
.PHONY: mirror-ro
.PRECIOUS: $(MIRRORDIR)/ro.owl
mirror-ro: | $(TMPDIR)
	if [ $(MIR) = true ] && [ $(IMP) = true ]; then curl -L $(OBOBASE)/ro.owl --create-dirs -o $(MIRRORDIR)/ro.owl --retry 4 --max-time 200 &&\
		$(ROBOT) convert -i $(MIRRORDIR)/ro.owl -o $@.tmp.owl && \
		$(ROBOT) remove -i $@.tmp.owl --base-iri http://purl.obolibrary.org/obo/BFO_ --base-iri http://purl.obolibrary.org/obo/RO_ --axioms external --preserve-structure false --trim false -o $@.tmp.owl &&\
		mv $@.tmp.owl $(TMPDIR)/$@.owl; fi


## ONTOLOGY: bfo
.PHONY: mirror-bfo
.PRECIOUS: $(MIRRORDIR)/bfo.owl
mirror-bfo: | $(TMPDIR)
	if [ $(MIR) = true ] && [ $(IMP) = true ]; then curl -L $(OBOBASE)/bfo.owl --create-dirs -o $(MIRRORDIR)/bfo.owl --retry 4 --max-time 200 &&\
		$(ROBOT) convert -i $(MIRRORDIR)/bfo.owl -o $@.tmp.owl &&\
		mv $@.tmp.owl $(TMPDIR)/$@.owl; fi


## ONTOLOGY: pato
.PHONY: mirror-pato
.PRECIOUS: $(MIRRORDIR)/pato.owl
mirror-pato: | $(TMPDIR)
	if [ $(MIR) = true ] && [ $(IMP) = true ] && [ $(IMP_LARGE) = true ]; then curl -L $(OBOBASE)/pato.owl --create-dirs -o $(MIRRORDIR)/pato.owl --retry 4 --max-time 200 &&\
		$(ROBOT) convert -i $(MIRRORDIR)/pato.owl -o $@.tmp.owl && \
		$(ROBOT) remove -i $@.tmp.owl --base-iri $(URIBASE)/PATO --axioms external --preserve-structure false --trim false -o $@.tmp.owl &&\
		mv $@.tmp.owl $(TMPDIR)/$@.owl; fi

ALL_MIRRORS = $(patsubst %, $(MIRRORDIR)/%.owl, $(IMPORTS))
MERGE_MIRRORS = true

$(MIRRORDIR)/merged.owl: $(ALL_MIRRORS)
	if [ $(IMP) = true ] && [ $(MERGE_MIRRORS) = true ]; then $(ROBOT) merge $(patsubst %, -i %, $^) -o $@; fi
.PRECIOUS: $(MIRRORDIR)/merged.owl


$(MIRRORDIR)/%.owl: mirror-% | $(MIRRORDIR)
	if [ $(IMP) = true ] && [ $(MIR) = true ] && [ -f $(TMPDIR)/mirror-$*.owl ]; then if cmp -s $(TMPDIR)/mirror-$*.owl $@ ; then echo "Mirror identical, ignoring."; else echo "Mirrors different, updating." &&\
		cp $(TMPDIR)/mirror-$*.owl $@; fi; fi

matentzn

LGTM. I may note that there is a tiny risk here that a custom pipeline depends on the mirror-% goal, but that would be in any case bad design.

Merging it now into dev and making sure any existing pipeline remains unaffected.

EDIT: you had already merged it and I had already run some tests. :P Looking good so far! Will approve as soon as I have run at least 5 ontology pipelines. I have finished 2.

gouttegd · 2024-01-05T12:32:41Z

I may note that there is a tiny risk here that a custom pipeline depends on the mirror-% goal, but that would be in any case bad design.

There is one occurrence that I know of in the Uberon custom Makefile: we re-define the mirror/ro.owl rule to add “OBO shorthands” to the mirror – whether adding such shorthands is something that we should really do is a good question in itself, but the important point here is that the re-definition does depend on mirror-ro:

# We add OBO shorthands to the RO mirror before merging it with the other mirrors
# FIXME: https://github.com/obophenotype/uberon/issues/3016
mirror/ro.owl: mirror-ro | $(MIRRORDIR)
        if [ $(MIR) = true ] && [ $(IMP) = true ]; then $(OWLTOOLS) $(TMPDIR)/mirror-ro.owl --add-obo-shorthand-to-properties -o $@ ; fi

After this PR is merged and when Uberon will be next updated with the latest ODK workflows, that rule above will need updating as well to adhere to the new principle of using Make conditionals rather than shell conditionals. I can take care of that of course.

gouttegd · 2024-01-05T12:33:42Z

you had already merged it

What the… I don’t recall at all having merged it to dev. I might have been more tired than I thought at the end of December. ^^'

matentzn

My three tests cannot find anything wrong with this, but have have forgotten to update CHANGES.md, and this change should probably get document with more than a link to a PR.

gouttegd · 2024-01-08T09:33:19Z

Not sure a changelog entry is warranted. This is purely an internal change. It may affect custom pipelines that re-defines or re-uses standard workflow targets, but you could say that for basically all internal changes. Users should be aware that the moment they start doing this kind of things in their $(ONT).Makefile, there is always a risk that something would break at the next update.

matentzn · 2024-01-08T11:48:49Z

Ok!

anitacaron assigned gouttegd Jan 3, 2024

matentzn reviewed Jan 5, 2024

View reviewed changes

Merge branch 'master' into fix-mirroring-pipeline

7a93b15

matentzn approved these changes Jan 7, 2024

View reviewed changes

gouttegd merged commit 27adfa9 into master Jan 8, 2024
1 check passed

gouttegd deleted the fix-mirroring-pipeline branch January 8, 2024 13:25

gouttegd mentioned this pull request May 17, 2024

Download component from source url workflow can't find file to copy #1060

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow refreshing the mirrors under IMP=false. #973

Allow refreshing the mirrors under IMP=false. #973

gouttegd commented Dec 20, 2023

gouttegd commented Dec 20, 2023

matentzn left a comment •

edited

Loading

gouttegd commented Jan 5, 2024

gouttegd commented Jan 5, 2024

matentzn left a comment

gouttegd commented Jan 8, 2024

matentzn commented Jan 8, 2024

Allow refreshing the mirrors under IMP=false. #973

Allow refreshing the mirrors under IMP=false. #973

Conversation

gouttegd commented Dec 20, 2023

gouttegd commented Dec 20, 2023

matentzn left a comment • edited Loading

Choose a reason for hiding this comment

gouttegd commented Jan 5, 2024

gouttegd commented Jan 5, 2024

matentzn left a comment

Choose a reason for hiding this comment

gouttegd commented Jan 8, 2024

matentzn commented Jan 8, 2024

matentzn left a comment •

edited

Loading